[cloud provider] Add AWS EC2 instance id semantic convention #600
Conversation
| When both `host.id` and `aws.ec2.instance.id` are present, | ||
| they SHOULD be equal. |
There was a problem hiding this comment.
Could this also be phrased as follows to avoid duplication?
| When both `host.id` and `aws.ec2.instance.id` are present, | |
| they SHOULD be equal. | |
| When the value in `host.id` is the same value as the `instance-id` | |
| returned by the metadata endpoint, only `host.id` should be set. |
There was a problem hiding this comment.
Per #576 (comment) I believe this is not what we do in similar cases such as with service.instance.id and k8s.pod.name. I think the situation is very similar here and we should do the same.
There was a problem hiding this comment.
It certainly makes the logic for both setting and reading the attribute easier. The duplicated string should in transport be taken care of by compression and also backends storing it would be able to deduplicate it as well.
I don't think we ever made a proper policy decision on whether to favor simplicity (for both instrumentation and querying) or deduplication but I believe either is a viable approach to take.
@open-telemetry/specs-semconv-approvers WDYT?
There was a problem hiding this comment.
I don't think we ever made a proper policy decision on whether to favor simplicity (for both instrumentation and querying) or deduplication but I believe either is a viable approach to take.
fwiw, we abandoned de-duplication in network.peer.address etc (328a2c6) because it made it hard to know whether an attribute is missing because it was a duplicate or because the particular instrumentation doesn't capture it
There was a problem hiding this comment.
I think this moves in a direction I'm hoping to push resource-detection -
That is a division of responsibilities. An AWS detector would be responsible for this attribute but NOT host.id.
Instead a Host Detector would generically detect host attributes as best as possible across known host-lookup mechanisms.
I'm writing up thoughts on this now, but I'd be in favor of this PR
There was a problem hiding this comment.
I'm writing up thoughts on this now, but I'd be in favor of this PR
@jsuereth Do you have any updates on this?
There was a problem hiding this comment.
I'm supportive of the addition as originally proposed by @mx-psi
Co-authored-by: Armin Ruech <[email protected]>
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
Reopening, waiting on @arminru's input above |
|
Hi @mx-psi ! We changed how the CHANGELOG.md is managed. Please take a look at https://github.com/open-telemetry/semantic-conventions/blob/main/CONTRIBUTING.md#adding-a-changelog-entry to see what needs to be done. Sorry for the disruption. |
|
Thanks @joaopgrassi, addressed. It looks like there is some sort of issue with the changelog check though that is (AFAICT) unrelated to this PR: |
|
Filed #739 for the general issue I am trying to solve by adding this. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
| brief: > | ||
| Resources used by Amazon Elastic Compute Cloud (Amazon EC2). | ||
| attributes: | ||
| - id: instance.id |
There was a problem hiding this comment.
@mx-psi Can you move the definition of this attribute to the registry and use a reference here? (see https://github.com/open-telemetry/semantic-conventions/pull/434/files for an example)
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
@mx-psi could you please update this PR to have all attributes defined in the registry and autogenerated and use only refs outside of the registry? |
|
I'd like to get a wider discussion on different attributes we have for I think we should either wait for the Resource project to start and get consensus in that WG or at least discuss it in the SemConv SIG meeting. |
Should we close this PR then for now? Not sure what the implications are for this PR |
Please don't close it :) I'm suggesting to join Semantic Conventions meeting on Mon 8am to discuss how we should treat all the instance ids. The meetings details are available in https://github.com/open-telemetry/community |
|
It's usually hard for me to join this meeting, but I may be able to join on Monday. |
|
Based on #576 (comment), I am going to hold off from further making changes on this PR until the Entities SIG gets up to speed |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
joaopgrassi
added
the
never stale
| @@ -0,0 +1,17 @@ | |||
| groups: | |||
| - id: aws.ec2 | |||
| prefix: aws.ec2 | |||
There was a problem hiding this comment.
Please stop using prefix.
|
Given the status of Entities WG and updates to semconv, this PR can now make progress and be submitted. It has the following:
Thanks and sorry for the long delay! |
Thank you for the update! I will update this PR then (but it may take me some time to get back to it) |
Changes
Add semantic convention for AWS EC2 instance id. This is similar to #15, and is inspired by #576, to ensure that the EC2 instance id can be correctly identified despite ambiguity in the
host.idconvention meaning.Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.
Merge requirement checklist